Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add correct column metadata to all warning checks #1127

Merged

Conversation

NJichev
Copy link
Contributor

@NJichev NJichev commented Apr 29, 2024

Hey 👋, I was working on next-ls code actions from credo when we noticed that the column information is missing.
This branch is a follow up on #1126. The main use case is generating a correct code action if there are two warnings on the same line. Without the column information 2 different warnings will map to the same AST when searching for it.

I kinda picked the column arbitrarily, so ping me if we should take it from another AST node, but generally I tried to follow the following rules:

  • If the warning check is about a function call use the column from the start of the module name |Enum.count(...)
  • If the warning check is about an operator use the operator start for the column(x |and x instead of the right/left hand side)
  • If the warning check is an erlang function use the column from the function (erlang.|function since the AST for the erlang bit is different and has no column information)

Copy link
Owner

@rrrene rrrene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great. Just a minor nitpick and we have to find out why the column info seems to be off for Elixir versions 1.11 and 1.12 ...

end

defp get_forbidden_call(ast, acc) do
{ast, acc}
end

defp issues_for_call(attribute, {call, trigger}, meta, issue_meta, issues) do
defp issues_for_call(attribute, {call_meta, call, trigger}, _meta, issue_meta, issues) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are ignoring _meta, we should probably not pass it on line 45.

@NJichev
Copy link
Contributor Author

NJichev commented Apr 29, 2024

So diffing elixir 1.13 and 1.12 doesn't reveal at first glance what changed between the two versions.
Checking the test examples I found that all the errors are coming from function calls coming from the erlang std library, i.e:

:erlang.open_port(...)
:os.cmd(...)

@rrrene
Copy link
Owner

rrrene commented Apr 30, 2024

Yeah. I just pushed some "checks" to the testing pipeline that make sure the trigger, line and column info is actually consisistent.

Could you rebase against master?

@NJichev NJichev force-pushed the add-correct-column-reporting-to-warning-checks branch from b1ef181 to 47b2000 Compare April 30, 2024 20:43
@rrrene
Copy link
Owner

rrrene commented Apr 30, 2024

Okay, so the new pipeline is showing some deeper issues: https://github.com/rrrene/credo/actions/runs/8901349869/job/24445604486?pr=1127

We have to provide an accurate column without changing the trigger (the trigger being the String to be highlighted by tooling like an editor).

@NJichev
Copy link
Contributor Author

NJichev commented Apr 30, 2024

Hmm I don't think I've touched any of the trigger code, I'll take a look tomorrow morning

@rrrene
Copy link
Owner

rrrene commented May 1, 2024

I probably did not phrase that in a good way, apologies.

The trigger is the string triggering the issue, e.g. ":os.cmd". The column has to be the start of that string in the source code.

This PR wants to fix instances where the column is wrong when there are two instances of the same call on the same line, which is super good 👍

But we cannot pick the column arbitrarily, because it has to be the actual start of the trigger 🙂

What I meant with my last comment is that it is not an option to change the trigger from ":os.cmd" to "cmd", because that would potentially break backwards compatibility and the intent of the check in question.

@NJichev
Copy link
Contributor Author

NJichev commented May 1, 2024

Maybe in this case I could get the module name and calculate it's string length and remove that from the column length to fix it, but it still doesn't cover the case where the AST is off by 1 on older versions of Elixir.
I could either compile a different version of the code for older versions to make it consistent if you think that's okay?
Another approach would be to just make this elixir version check in the tests and leave it like that?

@rrrene
Copy link
Owner

rrrene commented May 1, 2024

Let's ignore the off-by-one thing for Elixir 1.11 and 1.12 for now (I can deal with that after merging).

Our main focus should be to get the correct column for the existing trigger, as that is the primary improvement this PR brings 👍

@NJichev
Copy link
Contributor Author

NJichev commented May 1, 2024

Hey I fixed the Erlang stdlib function columns by just calculating the string length of the module. Also fixed some of the triggers that were producing warnings to match the real trigger.

Do you have any suggestions on how to fix the other 3 triggers that mismatch the actual code and column that are producing the warnings in the tests:

  • Enum empty check - here I return the column to be the start of the operator, I can make it be the Enum.count or length there and that would make it match the trigger.
  • Missing metadata key in logger config - so the column produced here is the start of the logger statement Logger.info(..., key: :foo) while the trigger is just "key" in this example - here I'm not exactly sure what's the most appropriate.
  • Forbidden Module check - here the warning comes from grouped aliases while the trigger itself is the fully qualified module name, so this warning won't be easily fixable, maybe remove the fully qualified name from the trigger, but that could be confusing as well.

So the last 2 can maybe be left as is. The empty enum check I've left it to follow the operator semantics above, but am open to changing.

@rrrene
Copy link
Owner

rrrene commented May 1, 2024

So the last 2 can maybe be left as is.

What do you mean? We can not include a wrong column that is not the start of the trigger in the source code.

In your first example, the trigger is actually Enum.count and not some kind of operator (I would also say that is the case from a educational point of view: the problem is not that you are using ==, the problem is that you are using Enum.count when you don't need to).

In the second example, of course the problem is the metadata entry in the keyword list (named :key in the test, not a good name I have to admit) and not the call to a Logger function.

Of course, in the special case with the grouped aliases for the Forbidden Module check, the trigger is actually wrong, because the string does not appear in the source code. So we have to change the (incorrect) trigger in this case (or you could simply revert your changes to this check and we deal with it in the future).

@NJichev
Copy link
Contributor Author

NJichev commented May 2, 2024

Hey 👋
I adjusted the expensive enum check column to get it from the correct operator side instead of using the line;col of the operator. Also fixed the trigger for the forbidden module to be the short name when in a group alias since the trigger is wrong otherwise and won't match the source code(did a small refactor there).

Decided to revert the column info for the missing metadata key in the Logger config since it's not so trivial to calculate the right column info for the starting position of the key in question. Maybe I could do it in a separate PR by using Macro.to_string instead of doing it by hand on the AST.

Copy link
Owner

@rrrene rrrene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are getting there. With one or two of the "non-necessary refactorings" I am afraid that this PR got worse, so let's discuss/revert 👍

@@ -27,28 +27,37 @@ defmodule Credo.Check.Warning.LeakyEnvironment do
Credo.Code.prewalk(source_file, &traverse(&1, &2, issue_meta))
end

@offset 2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give this a less confusing name: @colon_and_dot_length

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw that there's a backports module, should I move the attribute there as a function and compile a different version for the older elixir versions to make the tests pass?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned I will deal with that later. Especially because it won't be a problem for long, since we can stop supporting 1.12 as soon as Elixir 1.17 drops.

Enum.map(modules, fn key -> {Name.full(key), nil} end)
Map.new(modules, fn module ->
full = Name.full(module)
{full, "The `#{Name.full(module)}` module is not allowed."}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose to put the fallback message here instead of where it was? To me seems unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I did it because issue_for was getting too many arguments since I would need the trigger to be the short name in some cases, and then I'd need to pass in the full name as well in this scenario. I can revert it though

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can tolerate 5, 6 or even 7 arguments in a function in functional programming 🙂

Especially if it makes the flow & origin of information more clear.


defp issue_for(issue_meta, meta, trigger, message) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again: Why the refactor to get the message (and trigger) as an argument instead of looking it up here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trigger would need to be different if it's coming from a group alias, while the message is always based on the full name so far, since I didn't know if changing the message would be some kind of a compatibility problem decided to keep it consistent as it was. But I don't mind reworking it like this if you prefer?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the trigger is different for different scenarios, we can pass it in.

Comment on lines 69 to 70
# offset 2 characters for the dot call and the atom syntax
@offset 2
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above: please rename and delete the comment explaining the name

"""
|> to_source_file
|> run_check(@described_check)
|> assert_issues(fn [one, two] ->
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are these list items named in order here and named in reverse order on many other tests? Please be consistent.

"""
|> to_source_file
|> run_check(@described_check)
|> assert_issues(fn [one, two] ->
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

Enum.reduce(aliases, issues, fn {:__aliases__, meta, module}, issues ->
full_name = Name.full([base_alias, module])

if found_module?(full_name, forbidden_modules) do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this exactly the same as if forbidden_modules[full_name] do?

Copy link
Contributor Author

@NJichev NJichev May 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an artifact of the map initializing with nil values at first, while is_map_key would return true here.

@NJichev
Copy link
Contributor Author

NJichev commented May 2, 2024

Reverted the changes on the forbidden module check and just added a trigger as a variable + reduce one pass with enum.

rrrene added a commit that referenced this pull request May 4, 2024
@rrrene rrrene merged commit cdfd789 into rrrene:master May 4, 2024
16 of 21 checks passed
@rrrene
Copy link
Owner

rrrene commented May 4, 2024

@NJichev Thx for this! It highlighted several pain points in Credo's codebase 👍

FYI: I tried to simplify your PR further when merging on the command line: ba4f622

@NJichev
Copy link
Contributor Author

NJichev commented May 4, 2024

Thanks for guiding me through ❤️

@NJichev NJichev deleted the add-correct-column-reporting-to-warning-checks branch May 7, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants